Multiple text formats for symbols via "format" expression #6994

ChrisLoer · 2018-07-19T21:42:33Z

This PR introduces a format expression that creates a new formatted text type, which can be passed to the text-field property of symbols. This is the first part of addressing #4366 (aka "multi-icon/multi-text symbols").

"text-field": ["format",
    ["get", "name_en"], { "font-scale": 1.2 },
    "\n", {},
    ["get", "name"], { 
         "font-scale": 0.8,
         "text-font": ["literal", [ "DIN Offc Pro Italic", "Arial Unicode MS Regular" ]]
    }
]

The scope for this PR is to support two formatting options:

font-scale: a ratio to the text-size of the parent symbol. Defaults to 1. This must be expressed as a ratio in order for zoom-dependent symbol resizing to work effectively.
text-font: if present, overrides the base font of the symbol.

Further layout and paint properties will go in separate PRs.

This branch's current version of debug/debug.html uses a locally stored style sheet that exercises the variable formatting for both place-city-lg-s and road labels.

TODO

Implement style-supporting version of BiDi algorithm in mapbox-gl-rtl-text plugin
Align baselines for mixed font sizes on same line
- Now that baselines are aligned, fix the calculation of the bounding box for the shaping!
~~Switch internals from labeling each character with a section to tracking sections with "ranges"?~~
Get concatenation of mixed string/Formatted to automatically coerce to Formatted
~~Reduce fontStack tracking overhead (this might not actually be a big deal, no immediately discernible difference shows up in the Layout benchmark)~~
Fix broken tests

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
post benchmark scores
manually test the debug page
tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

/cc @anandthakker @hannahgelfond @samanpwbb @nickidlugash

jfirebaugh · 2018-07-19T21:56:41Z

We currently rely on static analysis of the style to determine which font stacks are used, so that they can be cached for offline use (mapbox/mapbox-gl-native#9939) or evicted from memory when no longer needed (mapbox/mapbox-gl-native#12388). Is that still possible under this proposal?

ChrisLoer · 2018-07-19T22:11:03Z

Is that still possible under this proposal?

Limiting the text-font format option to be a non-feature-dependent expression sounds fine, although I'm not sure how you do that nested within another expression (for instance, we'd have to forbid the "var" expression too right?). With that limitation, can't we just iterate through every "formatted" expression (regardless of whether it's on a branch that will get used) and analyze its text-font the same way we do for the root level text-font?

andrewharvey · 2018-07-20T02:13:18Z

The scope for this PR is to support two formatting options:

font-scale: a ratio to the text-size of the parent symbol. Defaults to 1. This must be expressed as a ratio in order for zoom-dependent symbol resizing to work effectively.
text-font: if present, overrides the base font of the symbol.

Is the thinking to extend this to include text-color, text-opacity, text-halo-* which would make #4366 (comment) possible?

ChrisLoer · 2018-07-20T03:57:08Z

Is the thinking to extend this to include text-color, text-opacity, text-halo-*

Yes, although not in this PR. Layout properties are a nice place to start because all of the layout information is already encoded per-glyph in the layout vertex buffers. To make this work with paint properties, we'll have to use something like the existing DDS paint property binders, even if the paint properties aren't actually data driven.

anandthakker · 2018-07-20T13:00:44Z

@ChrisLoer @jfirebaugh Yeah, I think the static analysis for text-font is still possible, it'll just require an approach that's more specialized than the current, general possibleOutputs approach.

we'd have to forbid the "var" expression too right?

I don't think we would -- when we're traversing the tree, any instance of "var" can just be dereferenced to the expression that variable is bound to.

ChrisLoer · 2018-07-20T21:46:59Z

The required mapbox-gl-rtl-text PR is here: mapbox/mapbox-gl-rtl-text#10

I'm currently using an interface where style sections are identified by an integer, and we annotate text strings by passing them around with a 1:1 matching array of style integers (one per character). It looks funky and duplicates a bunch of data in the common case, but since most of the logic involved wants to work character-by-character, it felt like a natural fit, and I doubt the overhead is that high... but it still looks odd and I'm waiting for someone to tell me I should do it differently. ;)

ChrisLoer · 2018-07-23T23:15:48Z

@anandthakker I made the change so that we try doing coercions for all possible overloads before giving up -- that makes ["concat", ["formatted" ...], "some plain string"] work as we want, but it also made four of the expression tests go from expected parse failure to success. It seemed reasonable to just update them, but I haven't thought through how this relaxation might be a breaking change. It seems to fit in with the other coercion-relaxation changes you've been working on. I'm not sure how this will interact with your changes at #6961.

Separate issue: there are some subtle differences between what font-scale means and what people might expect if they're thinking of it just as a multiplier of text-size. Most of these differences only really become apparent when you're using really big scales (beyond what I'd expect to be reasonable?). Here's an example of mixing scales 8 and 0.5:

You can see the halo is way too big for the scaled up text. I don't think there's an easy way to fix this and keep a single draw call without doing something like the paint property binder approach. It's also not very noticeable at more reasonable scales, so I'm not too worried
You can see that even though the line heights are adjusted based on scale, the descender of the gargantuan "p" drops over the small line. This is probably something we could do a better job of, I just haven't figured out a simple way to do it -- the logic is awkward because when we're doing layout we don't actually know the baseline of the glyphs we're scaling. But again, at more reasonable scales it doesn't look too bad.

For comparison, here's a scale 1.5 to .5:

anandthakker · 2018-07-24T16:12:23Z

@ChrisLoer the compound expression parsing change looks good. The affected existing tests are getting updated in #6961 anyway -- maybe just rebase onto there instead of updating them here?

ChrisLoer · 2018-07-24T23:54:13Z

@anandthakker Writing tests and validation code brought up two more little challenges:

What should the type of text-field in v8.json be? Right now I have it as "formatted" which is implemented as "string or expression". But ideally maybe we'd be able to say something like string | formatted in v8.json so that the docs are clearer, and automatically handle that in the style validation? Also not sure if I need to do anything up front to validate that the expression actually evaluates to something of Formatted type
We don't have a mechanism for overloading the declared return type of concat right now, which doesn't actually cause any problems at run time, but it does mean that when we create a Literal for the result, we tag it as a "string" which... is unexpected. Is there any reason we can't add support for changing the return type based on the overload?

ChrisLoer · 2018-07-25T18:00:42Z

Just talked to Anand, he gave helpful advice:

For the type of text-field in v8.json, let's just go with formatted for the purposes of documentation, along with an explanation of how a plain string gets coerced to default formatting options. It doesn't really matter too much under the hood when/where the coercion happens.

For concat, we can expand the type system to support overloads with different return types. But we could try a different approach, where instead of overloading concat, we turned formatted into an expression that takes a variable number of arguments, so you'd just build a single formatted string in one expression rather than concatenating individually formatted strings. e.g.:

Current implementation

["concat",
  ["formatted", "foo", { "font-scale": 1.5 }],
  "\b",
  ["formatted", "bar", { "font-scale": 0.5 }]
]

would become:
Alternate implementation

["formatted",
   "foo", { "font-scale": 1.5 },
   "\n", {},
   "bar", { "font-scale": 0.5}
]

The alternate approach is a bit more concise, it's a little more explicit about "default formatting", and I think it might feel more natural? On the con side, it pushes us away from the pattern "you can use a formatted like a string wherever there's an analogous operation".

@nickidlugash or @samanpwbb, any opinions on the two options?

anandthakker · 2018-07-25T18:12:02Z

On the con side, it pushes us away from the pattern "you can use a formatted like a string wherever there's an analogous operation".

I lean towards the alternate approach. This downside doesn't seem too bad, since formatted text is very likely to be used as the final output of an expression rather than an intermediate value.

By the way -- what about "format" rather than "formatted"? It's a little more consistent with, e.g., "concat"

nickidlugash · 2018-07-26T23:25:07Z

@ChrisLoer I think the alternative approach looks good.

Just so I can better envision how the syntax would be used in practice, can you confirm whether the style definitions based on this syntax would look more like this:

["formatted",
   "foo", {},
   "\n", {},
   "bar", { "font-scale": 0.5}
]

or if "foo" was just a literal:

["formatted",
   "foo \n", {},
   "bar", { "font-scale": 0.5}
]

rather than the example above, since it seems like it would usually make sense to use the default styling on as many of the strings as possible?

ChrisLoer · 2018-07-26T23:40:52Z

@nickidlugash Yeah I think you would usually have at least one section that used the default options (e.g. ..., {},), because why would you set the default formatting to something that you weren't actually going to use?

In terms of expression parsing, we could support omitting the options entirely, so it would instead be something like:

["formatted",
   "foo \n",
   "bar", { "font-scale": 0.5}
]

That might look more concise/prettier, at the expense of making the expression a little less explicit?

nickidlugash · 2018-07-27T00:02:09Z

That might look more concise/prettier, at the expense of making the expression a little less explicit?

I think from a user perspective, this is simpler to reason about, though the more explicit version seems fine, too.

By the way -- what about "format" rather than "formatted"? It's a little more consistent with, e.g., "concat"

👍

ChrisLoer · 2018-07-27T16:44:49Z

👍 I'll implement:

["format",
   "foo \n", {}
   "bar", { "font-scale": 0.5 }
]

I don't have strong feelings on whether to make the default formatting an optional argument, but I don't think we currently have anything that follows the kind of "varargs with optional arguments" pattern so I'm leaning towards holding off.

@anandthakker after this change we'll no longer need the changes to allow argument coercion in overloads, since concat won't depend on it. Do you think we should keep those changes in place anyway?

ChrisLoer · 2018-07-27T19:51:15Z

Benchmarks look OK I think:

ChrisLoer · 2018-07-27T20:54:19Z

Tagging @mapbox/studio and @mapbox/maps-design: I think we've settled on a final syntax.

One thing @nickidlugash and I discussed is that we might subtly alter the behavior of the font scaling in future implementations. The current implementation depends on modifying the positions of the layout vertices, which has the effect of stretching out the halo as well. When we do the text-color implementation, we'll have to build code that automatically uploads DDS-style buffers with per-glyph text-color values. If that code turns out to be easy to work with and performant, we might want to use the same strategy for text-size (so instead of changing the positions of the glyph vertices, we'd have the shaders scaled them based on a per-glyph text-size value). That would get rid of the slight weirdness with text-halo-width -- I can't think of any more dramatic effect it would have.

anandthakker · 2018-07-30T20:11:55Z

after this change we'll no longer need the changes to allow argument coercion in overloads, since concat won't depend on it. Do you think we should keep those changes in place anyway?

Hmm, good question. I suppose we might as well keep it (and port to native)?

anandthakker

Implementation looks great to me!

anandthakker · 2018-08-01T12:13:27Z

build/generate-style-code.js

@@ -5,6 +5,7 @@ const fs = require('fs');
 const ejs = require('ejs');
 const spec = require('../src/style-spec/reference/v8');
 const Color = require('../src/style-spec/util/color');
+const {Formatted} = require('../src/style-spec/expression/definitions/formatted');


anandthakker · 2018-08-01T12:16:39Z

flow-typed/style-spec.js

@@ -222,7 +225,7 @@ declare type SymbolLayerSpecification = {|
        "icon-pitch-alignment"?: PropertyValueSpecification<"map" | "viewport" | "auto">,
        "text-pitch-alignment"?: PropertyValueSpecification<"map" | "viewport" | "auto">,
        "text-rotation-alignment"?: PropertyValueSpecification<"map" | "viewport" | "auto">,
-        "text-field"?: DataDrivenPropertyValueSpecification<string>,
+        "text-field"?: DataDrivenPropertyValueSpecification<FormattedSpecification>,


Heads up there may be some file location weirdness when you rebase onto master, due to #7031

anandthakker · 2018-08-01T12:24:29Z

src/style-spec/expression/definitions/formatted.js

+    font: Expression | null;
+}
+
+export class FormattedExpression implements Expression {


Maybe rename to just Format?

anandthakker · 2018-08-01T12:55:11Z

src/style-spec/expression/definitions/formatted.js

+            if (!text) return null;
+            const kind = text.type.kind;
+            if (kind !== 'string' && kind !== 'value' && kind !== 'null')
+                return context.error(`Formatted text type must be 'string', 'value', or 'null'.`);


🤔 should we just accept any type here?

Basically support anything that has a string coercion? I'm ambivalent here, but I'll follow your guidance since you've been so involved in the explicit vs. implicit coercion design choices.

My starting position would be:

It's easier to reason about the implementation without automatic coercion

I can't immediately think of a "gotcha" case where this would be super unergonomic. If you want to format a plain number, you'll get an error message, but it'll be pretty clear what the error message means and there's a straightforward coercion call to address it.

We can always relax the requirements in the future

anandthakker · 2018-08-01T12:56:20Z

src/style-spec/expression/definitions/formatted.js

+
+            let font = null;
+            if (options['text-font']) {
+                font = context.parse(options['text-font'], 1, ValueType); // Require array of strings?


Yeah, I'd say require an array of strings

I wasn't sure how to do this, it would be something like array(StringType)? Will that work as we expect if the members of the array are ValueType (i.e. parsing will coerce to StringType)?

Made the change.

anandthakker · 2018-08-01T13:04:02Z

src/style-spec/validate/validate.js

            valueSpec: valueSpec.type ? styleSpec[valueSpec.type] : valueSpec
        }));
+        if (!valid) {
+            console.log("not valid");
+        }


anandthakker · 2018-08-01T13:15:07Z

src/symbol/shaping.js

-function breakLines(text: string, lineBreakPoints: Array<number>) {
+class TaggedString {
+    text: string;
+    sectionIndex: Array<number>


// maps each character in 'text' to its corresponding entry in 'sections'

🤔 The Formatted and TaggedString types kind of evolved separately as I worked on the implementation, but in the final result they end up looking pretty darn similar. I wonder if it makes sense to combine them? I don't think the cost of converting between the two is a big deal, and TaggedString has a lot of shaping-focused details, so I guess my inclination is to leave as is (but also probably I'm just lazy so good to get another pair of eyes on the decision).

anandthakker · 2018-08-01T13:58:19Z

src/symbol/transform_text.js

+
+export default function(text: string | Formatted, layer: SymbolStyleLayer, feature: Feature) {
+    if (text instanceof Formatted) {
+        // OK to transform in place?


I think it's probably fine... especially since we probably want to move towards having text transforms happen via expressions rather than through text-transform anyway

Returns "formatted" type with formatting annotations applied to subsections. "text-field" now accepts formatted text, allowing symbols to use multiple fonts within the same label.

…tiple overloads. Support string->formatted coercion (currently unused).

Add test using new formatted RTL functionality.

- "FormattedExpression" -> "FormatExpression" (reasoning: the expression itself is a verb, the result is a noun) - Remove dead/debug code - Add comments

anandthakker

🎉

ChrisLoer added feature 🍏 under development 🚧 labels Jul 19, 2018

ChrisLoer mentioned this pull request Jul 20, 2018

Add support for running BiDi algorithm on text with styling annotations. mapbox/mapbox-gl-rtl-text#10

Merged

ChrisLoer force-pushed the formatted-text branch from b807828 to 1c43c07 Compare July 24, 2018 18:59

ChrisLoer force-pushed the formatted-text branch from 9a2c511 to be75fe9 Compare July 27, 2018 19:47

ChrisLoer removed the under development 🚧 label Jul 27, 2018

ChrisLoer changed the title ~~WIP: multiple text formats for symbols~~ Multiple text formats for symbols via "format" expression Jul 27, 2018

ChrisLoer requested a review from anandthakker July 27, 2018 20:45

anandthakker reviewed Aug 1, 2018

View reviewed changes

ChrisLoer added 3 commits August 1, 2018 12:39

Introduce "format" expression.

f11e6c7

Returns "formatted" type with formatting annotations applied to subsections. "text-field" now accepts formatted text, allowing symbols to use multiple fonts within the same label.

Support implicit argument coercions for compound expressions with mul…

c88b4dd

…tiple overloads. Support string->formatted coercion (currently unused).

Update to mapbox-gl-rtl-text 0.2.0 for formatted text support.

c742e35

Add test using new formatted RTL functionality.

Address review comments:

e9bbaac

- "FormattedExpression" -> "FormatExpression" (reasoning: the expression itself is a verb, the result is a noun) - Remove dead/debug code - Add comments

ChrisLoer force-pushed the formatted-text branch from cc46fea to e9bbaac Compare August 1, 2018 19:41

Require that text-font format argument be an array of strings.

d2e6dcc

anandthakker approved these changes Aug 8, 2018

View reviewed changes

ChrisLoer merged commit 2cd748a into master Aug 8, 2018

brunoabinader mentioned this pull request Aug 13, 2018

Multiple text formats for symbols via "format" expression mapbox/mapbox-gl-native#12612

Closed

ChrisLoer mentioned this pull request Aug 13, 2018

Multiple text formats for symbols via "format" expression (native port) mapbox/mapbox-gl-native#12624

Merged

6 tasks

jfirebaugh mentioned this pull request Sep 13, 2018

Auto-coerce top-level results and concat arguments to strings #7280

Merged

5 tasks

ChrisLoer mentioned this pull request Sep 20, 2018

Identity functions don't handle 'formatted' type #7311

Closed

ChrisLoer mentioned this pull request Oct 15, 2018

Support multi-icon symbols #4366

Open

ChrisLoer mentioned this pull request Nov 30, 2018

Limit SDF dowscaling #7652

Open

mourner deleted the formatted-text branch August 1, 2019 12:34

Multiple text formats for symbols via "format" expression #6994

Multiple text formats for symbols via "format" expression #6994

Conversation

ChrisLoer commented Jul 19, 2018 • edited

TODO

Launch Checklist

jfirebaugh commented Jul 19, 2018

ChrisLoer commented Jul 19, 2018

andrewharvey commented Jul 20, 2018

ChrisLoer commented Jul 20, 2018

anandthakker commented Jul 20, 2018

ChrisLoer commented Jul 20, 2018

ChrisLoer commented Jul 23, 2018

anandthakker commented Jul 24, 2018

ChrisLoer commented Jul 24, 2018 • edited

ChrisLoer commented Jul 25, 2018

anandthakker commented Jul 25, 2018

nickidlugash commented Jul 26, 2018

ChrisLoer commented Jul 26, 2018

nickidlugash commented Jul 27, 2018

ChrisLoer commented Jul 27, 2018

ChrisLoer commented Jul 27, 2018

ChrisLoer commented Jul 27, 2018

anandthakker commented Jul 30, 2018

anandthakker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker left a comment

Choose a reason for hiding this comment

ChrisLoer commented Jul 19, 2018 •

edited

ChrisLoer commented Jul 24, 2018 •

edited